Recover a diverged repository from a source replica - #1333
Conversation
Motivation:
The scoped read-only feature keeps an in-memory read-only cache and Micrometer
metrics (repository.read.only.count and repository.read.only) that are
maintained only by a repository listener on the status files under
<root>/dogma/dogma/status. Deleting a read-only repository or project never
removed its status entry, so it lingered as a phantom row on the Repository
Status page (with a "Make writable" action that then failed) and permanently
leaked the metrics.
Modifications:
- RepoStatusManager
- Hide soft-removed repositories/projects from readOnlyStatuses() and the
metrics via a live isActive() existence check. The status file is
preserved, so restoring the repository/project before purge brings its
read-only status back.
- Delete the status file(s) on purge (removeRepoStatus/removeProjectStatus)
and evict the in-memory cache directly, since the snapshot-based listener
cannot observe deletions.
- Harden isActive() against a concurrently removed project and make the
metrics refresh non-throwing.
- StandaloneCommandExecutor
- Refresh the read-only metrics on repository/project remove and unremove.
- Delete the status file(s) on purge, gated on the target actually being
removed (markForPurge is a no-op otherwise, which would silently defeat
read-only) and best-effort so a cleanup failure never fails the already
applied command.
- Tests
- RepoStatusManagerTest: soft-delete-hides / restore-shows / purge-cleans for
both repository and project scope.
- ServerStatusServiceTest: end-to-end delete-while-read-only over HTTP.
- RepositoryStatusMetricsTest (new): drives the real command executor and
asserts the repository.read.only gauge follows the repository lifecycle.
Result:
Deleting a read-only repository or project no longer leaves a phantom entry in
the read-only list or a leaked metric series, and restoring before purge
preserves the read-only status.
Motivation: Once a repository has diverged across replicas and been made read-only, there is no way to repair it: the ZooKeeper replication log cannot move a git ref backward and old entries are trimmed away. An operator needs a way to designate one replica's repository as the source of truth and make every other replica converge to it. Modifications: - Add RecoverRepositoryRequestCommand, a trigger that is applied as a no-op on every replica; the source replica will react to it in the replication layer. - Add RecoverRepositoryCommand, a self-contained command that carries the commits to replay. Each ReplayCommit holds the original author, timestamp, summary, detail, markup, a self-contained change set and the expected commit ID of the revision, so every replica converges to identical commit SHAs. - Dispatch both commands in StandaloneCommandExecutor. - Add RepositoryManager.recoverRepository(): force-move the git head back to the reset revision (new GitRepository.doForceRefUpdate() that allows non-fast-forward updates), reopen the repository so the commit-id database rebuilds from the git HEAD, replay the carried commits while asserting each commit ID, and swap the repository instance. Any mismatch rolls back to the original instance. A repository that is already converged (the source or a healthy replica) is left untouched, which also makes recovery idempotent. Result: - The building blocks for repository recovery are in place: applying a recovery command resets a diverged replica onto the source history and verifies convergence commit-by-commit. Not yet reachable from any endpoint; the ZooKeeper coordination and the admin API follow.
Motivation:
The recovery commands and the git reset-and-replay core existed but were not
reachable: nothing coordinated who builds the recovery payload in a ZooKeeper
cluster, and no endpoint let an operator start a recovery.
Modifications:
- Coordinate the recovery over the replication log, originated by the source:
- Add RecoveryPayloadBuilder, which builds a self-contained
RecoverRepositoryCommand from the local storage of the source replica via
the new RepositoryManager.buildRecoveryPayload() (per-revision history,
self-contained changes and the expected commit ID).
- When a replayed RecoverRepositoryRequestCommand names this replica as the
source, ZooKeeperCommandExecutor reacts off the replay thread by building
and originating the recovery command. A RecoverRepositoryCommand failure
already trips only that repository's read-only scope because the failure
handling maps any RepositoryCommand to its project/repository.
- Add POST /api/v1/projects/{p}/repos/{r}/recover (system administrators
only): rejected unless the server runs in replicated (ZooKeeper) mode, the
source server ID is part of the cluster, the repository is file-based, and
the repository is read-only - so no new commit can race the payload build.
If the request lands on the source replica the recovery is originated
directly (status COMPLETED); otherwise a request command asks the source
over the replication log (status REQUESTED).
- Add GET /api/v1/replicas returning the cluster roster from the static
replication configuration, marking the replica that served the request; an
empty list in standalone mode, which also gates the UI.
Result:
- An operator can repair a diverged repository with a single admin API call.
Verified end-to-end on a 3-replica cluster with a truly diverged replica:
both the direct and the request-and-react paths converge every replica back
to the source history, the repository stays read-only until made writable,
and subsequent pushes replicate cleanly.
Motivation: Repository recovery was only reachable through the admin REST API. An operator repairing a diverged repository needs to see which replicas exist, pick the source of truth and start the recovery from the web UI, with a strong guard against running such a destructive action by accident. Modifications: - Add a "Repository Recovery" settings tab, shown only to system administrators and only in replicated (ZooKeeper) mode, gated on the new GET /api/v1/replicas endpoint; visiting the page in standalone mode explains why the feature is unavailable. - Add a recovery form with project/repository auto-complete, a source-server select fed by the replica list (marking the serving replica), and a start revision input. - Guard the destructive action with a type-to-confirm modal that spells out what is rewritten and that commits existing only on non-source replicas are discarded. - Show the outcome inline: a failure is rendered inside the modal (which stays open) and a success as a persistent alert under the form, so the result does not depend on the transient toast alone. Toasts are still dispatched. - Add getReplicas / recoverRepository to the API slice. - Add a jest test covering the enablement rule, the type-to-confirm gate, the request payload, the inline failure and the persistent success message, and extend the repo-status Playwright spec with the standalone-mode gating. - Add ReplicatedShiroCentralDogmaTestServer (and a runTestReplicatedShiroServer task) that runs the Shiro test server in single-replica ZooKeeper mode, so replication-only UI can be developed and verified against a real replicated server. Result: - A system administrator can run a repository recovery from the web UI with clear, persistent feedback. Verified live against a replicated test server: tab gating, form flow, the read-only precondition rejection shown inline and a completed recovery reporting the converged head revision.
Motivation: A five-lens review of the recovery feature surfaced one correctness gap and several robustness, observability and consistency issues: - The read-only precondition was checked only at request time. A command that made the repository writable again (and any push after it) could land in the replication log between the payload build and the recovery apply; applying the reset would then silently discard those commits on every replica. - A reader blocked on the old repository instance during the in-place reset could wake up after the swap and read the rewritten commit-id database through the stale instance. - A failure in the rollback path could skip the write-lock release, and a synchronous executor failure in the source's recovery reaction was dropped without a log. Modifications: - Re-verify the read-only precondition when RecoverRepositoryCommand is applied, using only the replicated repository/project scope (RepoStatusManager.isRepoOrProjectReadOnly) so every replica decides identically; on the originating replica the recovery aborts before the command enters the log. - Fail fast a reader that was blocked on the old repository instance: mark the instance close-pending (new GitRepository.markClosePending) before releasing the write lock, and decouple close scheduling from the close-pending flag so the later close() still releases the resources. - Guard the rollback path so a failing close of the partially recovered instance cannot leak the old repository's write lock, log synchronous failures of the recovery reaction, reject a reset revision above the local head with an actionable message, and report a single-commit repository as having nothing to replay. - Type the recover endpoint response (RecoverRepositoryResponse) instead of an untyped map, and document the recovery constraints (force-push races, best-effort REQUESTED path, rolling upgrades) on the endpoint. - Web UI: clear the previous outcome when a new recovery is configured, label the confirmation input for screen readers, and reword the REQUESTED result to say the recovery is asynchronous and best-effort instead of implying the Repository Status page can verify convergence. - Tests: replay coverage now includes multi-file, JSON and removal commits and builds payloads through the production buildRecoveryPayload (plus bounds checks); JSON round-trips for the new commands; cluster tests for the replica roster, an out-of-range start revision, idempotent double recovery and the writable-abort re-check; a SettingView test pinning the tab-order invariant that keeps the hidden Recovery tab from misaligning the highlight. Result: - A recovery can no longer discard commits that raced it, stale reads through the swapped-out repository instance fail fast, and the operator-facing contract (typed response, honest REQUESTED wording, documented constraints) matches what the system actually guarantees.
Motivation: An adversarial review round (five reviewers, each attacking one invariant of the recovery feature) confirmed three exploitable gaps: - The apply-time read-only re-check reads an in-memory cache that is loaded by an asynchronously registered listener. A replica restarting with the recovery still in its replication backlog could replay it before the cache loaded, answer "writable" while the rest of the cluster answered "read-only", abort the recovery and poison-skip the log entry - ending up silently diverged behind an already-read-only flag. - A repository whose replay range contains transformer-written entries (the per-project internal dogma/meta repositories, e.g. metadata.json) can never converge: transformers store JSON without the trailing-newline text normalization, while the recovery payload and its replay both normalize, so the replayed commit ID always mismatches and recovery aborts after doing the work. - The recovery payload is unbounded. It crosses the replication log as a single entry and is materialized in memory by every replica, so recovering a large repository from an early revision could exhaust heaps cluster-wide, stall replication while replicas replay it, and bloat ZooKeeper. Modifications: - Make the read-only re-check trustworthy on a cold cache: a read-only answer from the cache is authoritative, but a writable answer is re-verified against the replicated status files, which reflect the replication-log position on every replica. - Reject recovery of internal repositories at the endpoint and hide them from the recovery form; mention the not-byte-reproducible cause in the commit-ID mismatch error. - Cap the recovery payload (10,000 commits / 64 MiB estimated content) with a clear message to recover from a later revision. - Refresh the replica roster whenever the recovery page or form mounts instead of trusting a session-old cache, guard against re-entrant submissions, and drop a stale comment about a removed gc lock. - Tests: a cold-cache manager must answer read-only from the stored status (reproduces the restart race); payload-cap boundaries; the endpoint rejects an internal repository on a live cluster. Result: - A restarting replica can no longer silently skip a recovery the rest of the cluster applied, guaranteed-to-fail recoveries of internal repositories are rejected up front, and a recovery can no longer take down the cluster by sheer payload size.
Motivation: A recovery requested through a non-source replica completes asynchronously and its failure is only reported in the source replica's log, so the operator must compare the repository head on every replica before making it writable again. The UI explained this but left the operator to figure out how. Modifications: - After a REQUESTED recovery, render a copy-pastable shell script next to the result: one curl per replica (built from the live replica roster and the page origin) that prints the repository's head revision, with the source replica marked, plus a reminder that failures appear only in the source replica's log. - Snapshot the recovered project/repository and the source server ID in the result state so the script always describes the recovery it belongs to. Result: - An administrator can verify cluster-wide convergence with a single pasted command block instead of hand-crafting per-replica requests.
…y UI Motivation: The verification script hard-coded a plain per-replica URL, which breaks on an HTTPS deployment (the certificate and any virtual-host routing are issued for the load balancer's name, not for a replica host). And a rejected recovery rendered the server's whole Java stack trace, which a system administrator always receives, burying the one sentence that explains the rejection. Modifications: - Keep this page's authority in each curl and dial the replica directly with --connect-to, so TLS validation and virtual-host routing keep working, with a note about `curl -k` as the alternative; default the port to Central Dogma's 36462 for http. - Show only the reason of a rejected recovery: drop the stack frames and the exception class that repeats the message, in the inline modal error and the toast alike. Result: - The verification script is usable as-is on an HTTPS cluster, and a rejected recovery reads as a single actionable sentence.
Motivation: The per-replica verification script kept the page's authority in every curl and dialed the replica with --connect-to, which made each line long and hard to read, and it rendered as plain unhighlighted text. Modifications: - Address each replica by its own host name. Over HTTPS that host is not covered by the certificate issued for the load balancer, so add -k there; over HTTP there is nothing to skip, so do not. - Syntax-highlight the script with Prism, as the variable view already does. Result: - The script is a short, readable, highlighted command per replica.
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Motivation: RecoveryStatus was nested in RecoverRepositoryResponse, so every use spelled out the enclosing class. Modifications: - Move RecoveryStatus into its own file. Result: - No behavioral change.
Motivation: The Recover button shared the wrapping row of the recovery form, so once the fields wrapped it ended up next to the start revision, floating in the middle of the second row. Modifications: - Give the action its own row, right-aligned, and top-align the fields so the inputs line up across a wrap. - Widen the start revision field so its helper text fits on one line. Result: - The form reads as fields first, then the action, at every width.
… purge
Motivation:
A second review round, this time with the rendered UI in hand, found that the
feature could tell an administrator a recovery had succeeded when it had not:
- The verification script compared head revisions. Replicas of a diverged
repository always report the same revision - that is what divergence is
here - so the script reported agreement in exactly the case it existed to
catch, and a rolled-back replica looked converged.
- COMPLETED was presented as "the repository converged", but it only means the
source replica originated the recovery; every other replica converges when
it replays it, exactly like REQUESTED. The verification script was also
withheld from that path.
- The purge-time status cleanup still trusted the in-memory cache, which is
the bug that was just fixed in the read-only re-check: a replica replaying a
purge before its status listener has loaded keeps the status file forever,
so its internal dogma repository silently falls behind the cluster.
Modifications:
- Add GET /api/v1/projects/{p}/repos/{r}/head (system administrators only),
returning the revision and the commit ID of the head on the replica that
served the request, and compare the commit ID in the verification script.
A revision cannot distinguish diverged replicas; a commit ID can.
- Say what actually happened: neither COMPLETED nor REQUESTED means the
cluster converged, and both now carry the verification script.
- Delete the status file on purge regardless of the cache, and enumerate a
project's status files from storage rather than the cache.
- Peel a RuntimeException in the read-only re-check: the storage lookup throws
synchronously, so a missing status storage was not recognised.
- Report an already-converged replica at INFO, so a recovery is not read
backwards from the logs; never dump a replication payload into a log line
(a recovery payload can be tens of megabytes); reject an internal repository
with 403 and a writable repository with 409, as the sibling endpoints do;
and keep a successful swap successful even if the listener handover fails.
- Build the recovery payload in one place: the endpoint now uses
RecoveryPayloadBuilder instead of repeating the reset-point and head rules.
- Fix the dark-mode script block, whose light Prism theme left the comments
unreadable and painted white boxes behind the operators; put the read-only
precondition above the action; reset the whole form after a recovery.
- Tests: the cluster test now asserts commit-ID convergence and that a
diverged replica shares the source's revision but not its commit; the
payload cap is exercised through buildRecoveryPayload instead of a static
helper; a cold-cache purge must still delete the status files; a replica
missing the reset base is rejected.
Result:
- An administrator can now actually prove a recovery converged, and a purge
replayed on a restarting replica no longer diverges its status storage.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
server/src/main/java/com/linecorp/centraldogma/server/internal/management/RepoStatusManager.java (1)
166-169: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueExtract the repeated
isActivefilter predicate.The same
.filter(state -> isActive(state.projectName(), state.repoName()))predicate is duplicated inreadOnlyStatuses(),refreshReadOnlyMetrics(), andactiveReadOnlyCount(). Consider a small private helper (e.g.activeStates()) returning the filtered stream to avoid triple duplication.♻️ Proposed refactor
+ private Stream<RepositoryState> activeStates() { + return statusMap.values().stream() + .filter(state -> isActive(state.projectName(), state.repoName())); + } + public List<RepositoryState> readOnlyStatuses() { - return statusMap.values().stream() - .filter(state -> isActive(state.projectName(), state.repoName())) - .collect(toImmutableList()); + return activeStates().collect(toImmutableList()); }Also applies to: 327-372
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/src/main/java/com/linecorp/centraldogma/server/internal/management/RepoStatusManager.java` around lines 166 - 169, Extract the duplicated active-state filtering from readOnlyStatuses(), refreshReadOnlyMetrics(), and activeReadOnlyCount() into a private helper such as activeStates(). Have the helper return the status stream filtered with isActive(state.projectName(), state.repoName()), and update all three callers to reuse it while preserving their existing terminal operations.server/src/main/java/com/linecorp/centraldogma/server/internal/replication/ZooKeeperCommandExecutor.java (1)
439-445: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winNarrow
replicationConfig()'s exposure of the cluster secret.
replicationConfig()returns the fullZooKeeperReplicationConfig, which carriescfg.secret()(the shared SASL secret used to auth the embedded ZooKeeper ensemble, perstartZooKeeper()). Today onlyServerStatusService.replicas()consumes it, and it immediately narrows to.servers(), so nothing leaks yet. But as a public getter it widens the surface for future misuse (e.g. a stray.toString()/log call) to expose the secret.Consider exposing only what callers actually need instead:
♻️ Narrower accessor
- /** - * Returns the replication configuration of this cluster. - */ - public ZooKeeperReplicationConfig replicationConfig() { - return cfg; - } + /** + * Returns the replicas of this cluster from the static replication configuration. + */ + public Map<Integer, ZooKeeperServerConfig> replicaServers() { + return cfg.servers(); + }I don't have visibility into
ZooKeeperReplicationConfig'stoString()/serialization to confirm whethersecret()is already redacted there — could you check?🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/src/main/java/com/linecorp/centraldogma/server/internal/replication/ZooKeeperCommandExecutor.java` around lines 439 - 445, Change ZooKeeperCommandExecutor.replicationConfig() to expose only the non-secret replication data required by ServerStatusService.replicas(), rather than returning the full ZooKeeperReplicationConfig containing cfg.secret(). Inspect ZooKeeperReplicationConfig’s toString/serialization behavior, and update the accessor and its caller to use a secret-free representation while preserving access to the configured servers.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@server/src/main/java/com/linecorp/centraldogma/server/internal/api/RepositoryServiceV1.java`:
- Around line 306-332: Update RepositoryServiceV1.head so the revision and
commit ID come from one consistent snapshot: obtain headRevision, then resolve
the commit through repository.commitIdDatabase().get(headRevision) or the
repository’s single locked accessor instead of calling jGitRepository().resolve
separately. Preserve the existing StorageException handling and no-head
validation, ensuring RepositoryHead always contains the commit corresponding to
headRevision.
In
`@server/src/main/java/com/linecorp/centraldogma/server/internal/storage/repository/git/GitRepositoryManager.java`:
- Around line 340-352: Update the expectedCommitId validation in the recovery
method to safely handle commitIdDatabase().get(revision) returning null before
calling name(). Represent the actual commit ID as null when no commit exists,
then reuse the existing mismatch comparison and StorageException diagnostic so
missing commits produce the intended commit-ID mismatch error instead of an NPE.
In
`@webapp/javaTest/java/com/linecorp/centraldogma/webapp/ReplicatedShiroCentralDogmaTestServer.java`:
- Around line 61-74: Update the main startup flow in
ReplicatedShiroCentralDogmaTestServer.main so cleanup for server1 and server2 is
installed before scaffold() runs, and ensure startup or scaffolding failures
close both servers immediately. Preserve the existing shutdown-hook cleanup for
normal process termination while preventing failed initialization from leaving
either replica running.
In `@webapp/src/dogma/features/api/apiSlice.ts`:
- Around line 501-502: Update the recovery mutation’s invalidatesTags
configuration to invalidate both the existing Repo and File tags, ensuring
getFiles, getFileContent, and getHistory caches refresh after repository history
is rewritten.
---
Nitpick comments:
In
`@server/src/main/java/com/linecorp/centraldogma/server/internal/management/RepoStatusManager.java`:
- Around line 166-169: Extract the duplicated active-state filtering from
readOnlyStatuses(), refreshReadOnlyMetrics(), and activeReadOnlyCount() into a
private helper such as activeStates(). Have the helper return the status stream
filtered with isActive(state.projectName(), state.repoName()), and update all
three callers to reuse it while preserving their existing terminal operations.
In
`@server/src/main/java/com/linecorp/centraldogma/server/internal/replication/ZooKeeperCommandExecutor.java`:
- Around line 439-445: Change ZooKeeperCommandExecutor.replicationConfig() to
expose only the non-secret replication data required by
ServerStatusService.replicas(), rather than returning the full
ZooKeeperReplicationConfig containing cfg.secret(). Inspect
ZooKeeperReplicationConfig’s toString/serialization behavior, and update the
accessor and its caller to use a secret-free representation while preserving
access to the configured servers.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 052052a1-08f0-441c-87f9-d5d1c1c0fee6
📒 Files selected for processing (42)
server/src/main/java/com/linecorp/centraldogma/server/CentralDogma.javaserver/src/main/java/com/linecorp/centraldogma/server/command/Command.javaserver/src/main/java/com/linecorp/centraldogma/server/command/CommandType.javaserver/src/main/java/com/linecorp/centraldogma/server/command/RecoverRepositoryCommand.javaserver/src/main/java/com/linecorp/centraldogma/server/command/RecoverRepositoryRequestCommand.javaserver/src/main/java/com/linecorp/centraldogma/server/command/ReplayCommit.javaserver/src/main/java/com/linecorp/centraldogma/server/command/StandaloneCommandExecutor.javaserver/src/main/java/com/linecorp/centraldogma/server/internal/api/RecoverRepositoryRequest.javaserver/src/main/java/com/linecorp/centraldogma/server/internal/api/RecoverRepositoryResponse.javaserver/src/main/java/com/linecorp/centraldogma/server/internal/api/RecoveryStatus.javaserver/src/main/java/com/linecorp/centraldogma/server/internal/api/RepositoryHead.javaserver/src/main/java/com/linecorp/centraldogma/server/internal/api/RepositoryServiceV1.javaserver/src/main/java/com/linecorp/centraldogma/server/internal/api/sysadmin/ReplicaInfo.javaserver/src/main/java/com/linecorp/centraldogma/server/internal/api/sysadmin/ServerStatusService.javaserver/src/main/java/com/linecorp/centraldogma/server/internal/management/RepoStatusManager.javaserver/src/main/java/com/linecorp/centraldogma/server/internal/replication/RecoveryPayloadBuilder.javaserver/src/main/java/com/linecorp/centraldogma/server/internal/replication/ReplicationLogContext.javaserver/src/main/java/com/linecorp/centraldogma/server/internal/replication/ZooKeeperCommandExecutor.javaserver/src/main/java/com/linecorp/centraldogma/server/internal/storage/repository/RepositoryManagerWrapper.javaserver/src/main/java/com/linecorp/centraldogma/server/internal/storage/repository/git/GitRepository.javaserver/src/main/java/com/linecorp/centraldogma/server/internal/storage/repository/git/GitRepositoryManager.javaserver/src/main/java/com/linecorp/centraldogma/server/storage/repository/RepositoryManager.javaserver/src/test/java/com/linecorp/centraldogma/server/command/RecoverRepositoryCommandTest.javaserver/src/test/java/com/linecorp/centraldogma/server/command/RecoverRepositoryRequestCommandTest.javaserver/src/test/java/com/linecorp/centraldogma/server/internal/api/RepositoryServiceV1Test.javaserver/src/test/java/com/linecorp/centraldogma/server/internal/api/RepositoryStatusMetricsTest.javaserver/src/test/java/com/linecorp/centraldogma/server/internal/api/ServerStatusServiceTest.javaserver/src/test/java/com/linecorp/centraldogma/server/internal/management/RepoStatusManagerTest.javaserver/src/test/java/com/linecorp/centraldogma/server/internal/replication/Replica.javaserver/src/test/java/com/linecorp/centraldogma/server/internal/replication/ZooKeeperRepositoryRecoveryIntegrationTest.javaserver/src/test/java/com/linecorp/centraldogma/server/internal/storage/repository/git/RecoverRepositoryTest.javawebapp/build.gradlewebapp/e2e/repo-status.spec.tswebapp/javaTest/java/com/linecorp/centraldogma/webapp/ReplicatedShiroCentralDogmaTestServer.javawebapp/src/dogma/features/api/apiSlice.tswebapp/src/dogma/features/settings/SettingView.tsxwebapp/src/dogma/features/settings/recovery/RecoverRepositoryForm.tsxwebapp/src/dogma/features/settings/recovery/RecoveryConfirmModal.tsxwebapp/src/dogma/features/settings/recovery/RecoveryDto.tswebapp/src/pages/app/settings/recovery/index.tsxwebapp/tests/dogma/feature/settings/RecoverRepositoryForm.test.tsxwebapp/tests/dogma/feature/settings/SettingView.test.tsx
| /** | ||
| * GET /projects/{projectName}/repos/{repoName}/head | ||
| * | ||
| * <p>Returns the head of the repository <em>on the replica that served the request</em>, identified by | ||
| * both its revision and its commit ID. Two replicas of the same repository always share a revision, | ||
| * even when their histories have diverged, so only the commit ID proves that they hold the same | ||
| * history. It is how an administrator confirms that a recovery converged before making the repository | ||
| * writable again. | ||
| */ | ||
| @Get("/projects/{projectName}/repos/{repoName}/head") | ||
| @RequiresSystemAdministrator | ||
| public RepositoryHead head(Repository repository) { | ||
| final Revision headRevision = repository.normalizeNow(Revision.HEAD); | ||
| final ObjectId commitId; | ||
| try { | ||
| commitId = repository.jGitRepository().resolve(Constants.R_HEADS + Constants.MASTER); | ||
| } catch (IOException e) { | ||
| throw new StorageException("failed to resolve the head commit of " + | ||
| repository.parent().name() + '/' + repository.name(), e); | ||
| } | ||
| if (commitId == null) { | ||
| throw new StorageException("no head commit in " + | ||
| repository.parent().name() + '/' + repository.name()); | ||
| } | ||
| return new RepositoryHead(headRevision, commitId.name()); | ||
| } | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
set -euo pipefail
# Map the relevant files and symbols first.
git ls-files | rg 'server/src/main/java/com/linecorp/centraldogma/server/internal/api/RepositoryServiceV1\.java|server/src/main/java/com/linecorp/centraldogma/server/internal/storage|server/src/main/java/com/linecorp/centraldogma/server/internal|common/src/main/java/com/linecorp/centraldogma/server'
echo '---'
ast-grep outline server/src/main/java/com/linecorp/centraldogma/server/internal/api/RepositoryServiceV1.java --view expanded
echo '---'
fd -a 'GitRepositoryManager.java|GitRepository.java|Repository.java|RepositoryHead.java' server src common .Repository: line/centraldogma
Length of output: 27101
🏁 Script executed:
set -euo pipefail
# Inspect the methods and lock usage in the storage layer.
for f in \
server/src/main/java/com/linecorp/centraldogma/server/internal/storage/GitRepositoryManager.java \
server/src/main/java/com/linecorp/centraldogma/server/internal/storage/GitRepository.java \
server/src/main/java/com/linecorp/centraldogma/server/internal/api/RepositoryServiceV1.java \
server/src/main/java/com/linecorp/centraldogma/server/internal/storage/Repository.java \
server/src/main/java/com/linecorp/centraldogma/server/internal/storage/RepositoryHead.java
do
if [ -f "$f" ]; then
echo "===== $f ====="
wc -l "$f"
fi
done
echo '--- GitRepositoryManager outline ---'
ast-grep outline server/src/main/java/com/linecorp/centraldogma/server/internal/storage/GitRepositoryManager.java --view expanded
echo '--- GitRepository outline ---'
ast-grep outline server/src/main/java/com/linecorp/centraldogma/server/internal/storage/GitRepository.java --view expandedRepository: line/centraldogma
Length of output: 731
🏁 Script executed:
set -euo pipefail
# Show the relevant methods and any lock usage around them.
for f in \
server/src/main/java/com/linecorp/centraldogma/server/internal/storage/GitRepositoryManager.java \
server/src/main/java/com/linecorp/centraldogma/server/internal/storage/GitRepository.java \
server/src/main/java/com/linecorp/centraldogma/server/internal/api/RepositoryServiceV1.java
do
echo "===== $f ====="
rg -n "normalizeNow|commitIdDatabase|jGitRepository|readLock|writeLock|resolve\\(|forceMoveMaster|recoverRepository|head\\(" "$f" || true
echo
doneRepository: line/centraldogma
Length of output: 1540
🌐 Web query:
"site:github.com linecorp centraldogma RepositoryServiceV1 head normalizeNow jGitRepository resolve Constants.R_HEADS MASTER"
💡 Result:
In the LINE Central Dogma project, the usage of jGitRepository.resolve(Constants.R_HEADS + "master") (often associated with normalizing or accessing the master branch) is a pattern used to interact with the underlying JGit storage layer [1]. The Central Dogma server uses a Repository interface to manage version-controlled, filesystem-like data [1]. This interface provides the jGitRepository method, which returns the underlying org.eclipse.jgit.lib.Repository object [1]. When developers interact with this JGit instance, they frequently use JGit's Constants.R_HEADS (which is the prefix "refs/heads/") [2] combined with a branch name, such as "master", to resolve the ObjectId of that specific branch [3]. The resolve(String) method in JGit is responsible for parsing revision strings and returning the corresponding ObjectId [3]. While Central Dogma contains various service classes like RepositoryServiceV1 (which manages API-level repository operations) [4][5], the specific invocation of normalizing branches or revisions via JGit constants typically happens at the storage layer where Central Dogma's internal repositories (implementations of the Repository interface) interface with JGit [1]. The normalizeNow method mentioned in your query is likely a reference to internal testing or repository management utility methods within the Central Dogma codebase where branch references are resolved and verified against the JGit state [6].
Citations:
- 1: https://line.github.io/centraldogma/xref/com/linecorp/centraldogma/server/storage/repository/Repository.html
- 2: https://archive.eclipse.org/jgit/site/6.1.0.202203080745-r/org.eclipse.jgit/apidocs/org.eclipse.jgit/org/eclipse/jgit/lib/Constants.html
- 3: https://download.eclipse.org/jgit/site/6.4.0.202211300538-r/apidocs/org/eclipse/jgit/lib/Repository.html
- 4: Scope replication-failure read-only to repo/project, not the whole server #1305
- 5: https://line.github.io/centraldogma/xref/com/linecorp/centraldogma/server/internal/admin/service/RepositoryService.html
- 6: Revert "Cache an entire repository to increase cache hit rate (#1098)" #1306
🏁 Script executed:
set -euo pipefail
# Map the actual files that exist in this repo.
for f in \
server/src/main/java/com/linecorp/centraldogma/server/internal/storage/repository/git/GitRepositoryManager.java \
server/src/main/java/com/linecorp/centraldogma/server/internal/storage/repository/git/GitRepository.java \
server/src/main/java/com/linecorp/centraldogma/server/storage/repository/Repository.java \
server/src/main/java/com/linecorp/centraldogma/server/internal/api/RepositoryHead.java
do
echo "===== $f ====="
wc -l "$f"
ast-grep outline "$f" --view expanded
echo
doneRepository: line/centraldogma
Length of output: 9398
🏁 Script executed:
set -euo pipefail
# Inspect the methods and lock usage in the actual files.
for f in \
server/src/main/java/com/linecorp/centraldogma/server/internal/storage/repository/git/GitRepositoryManager.java \
server/src/main/java/com/linecorp/centraldogma/server/internal/storage/repository/git/GitRepository.java \
server/src/main/java/com/linecorp/centraldogma/server/storage/repository/Repository.java \
server/src/main/java/com/linecorp/centraldogma/server/internal/api/RepositoryServiceV1.java
do
echo "===== $f ====="
rg -n "normalizeNow|jGitRepository|commitIdDatabase|readLock|writeLock|forceMoveMaster|recoverRepository|resolve\\(|head\\(" "$f" || true
echo
doneRepository: line/centraldogma
Length of output: 10321
🏁 Script executed:
set -euo pipefail
sed -n '300,340p' server/src/main/java/com/linecorp/centraldogma/server/internal/api/RepositoryServiceV1.java
echo '---'
sed -n '1,260p' server/src/main/java/com/linecorp/centraldogma/server/internal/storage/repository/git/GitRepository.java
echo '---'
sed -n '1,260p' server/src/main/java/com/linecorp/centraldogma/server/internal/storage/repository/git/GitRepositoryManager.java
echo '---'
sed -n '1,260p' server/src/main/java/com/linecorp/centraldogma/server/storage/repository/Repository.javaRepository: line/centraldogma
Length of output: 37618
🏁 Script executed:
set -euo pipefail
# Look for any revision-to-commit lookup helpers or atomic head accessors in the git repository code.
rg -n "CommitIdDatabase|commit id|commitId|getHead|headRevision|RepositoryHead|normalizeNow\\(|jGitRepository\\(" \
server/src/main/java/com/linecorp/centraldogma/server/internal/storage/repository/git \
server/src/main/java/com/linecorp/centraldogma/server/storage/repository \
server/src/main/java/com/linecorp/centraldogma/server/internal/apiRepository: line/centraldogma
Length of output: 42303
🏁 Script executed:
set -euo pipefail
# Read the specific implementations and surrounding lock usage.
sed -n '320,345p' server/src/main/java/com/linecorp/centraldogma/server/internal/storage/repository/git/GitRepository.java
echo '---'
sed -n '372,405p' server/src/main/java/com/linecorp/centraldogma/server/internal/storage/repository/git/GitRepository.java
echo '---'
sed -n '286,386p' server/src/main/java/com/linecorp/centraldogma/server/internal/storage/repository/git/GitRepositoryManager.java
echo '---'
sed -n '407,420p' server/src/main/java/com/linecorp/centraldogma/server/internal/storage/repository/git/GitRepositoryManager.javaRepository: line/centraldogma
Length of output: 9295
Potential race between headRevision and commitId reads server/src/main/java/com/linecorp/centraldogma/server/internal/api/RepositoryServiceV1.java:317-326 — normalizeNow(Revision.HEAD) and jGitRepository().resolve(...) read the head through separate paths, so head() can return a revision/commit pair from different moments if recovery or a force-push lands in between. Use commitIdDatabase().get(headRevision) or a single locked accessor so both fields describe the same commit.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@server/src/main/java/com/linecorp/centraldogma/server/internal/api/RepositoryServiceV1.java`
around lines 306 - 332, Update RepositoryServiceV1.head so the revision and
commit ID come from one consistent snapshot: obtain headRevision, then resolve
the commit through repository.commitIdDatabase().get(headRevision) or the
repository’s single locked accessor instead of calling jGitRepository().resolve
separately. Preserve the existing StorageException handling and no-head
validation, ensuring RepositoryHead always contains the commit corresponding to
headRevision.
| final String expectedCommitId = commit.expectedCommitId(); | ||
| if (expectedCommitId != null) { | ||
| final String actualCommitId = neo.commitIdDatabase().get(revision).name(); | ||
| if (!expectedCommitId.equals(actualCommitId)) { | ||
| throw new StorageException( | ||
| "commit id mismatch while recovering '" + | ||
| projectRepositoryName(repositoryName) + "' at " + revision + " (expected: " + | ||
| expectedCommitId + ", actual: " + actualCommitId + "). Revisions up to " + | ||
| resetToRevision + " may have diverged, or the content is not reproducible " + | ||
| "byte-identically (e.g. written by a content transformer); the repository " + | ||
| "was rolled back."); | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Unguarded .name() call can NPE instead of throwing the intended mismatch error.
Line 299-300 in this same method defends against commitIdDatabase().get(...) returning null before calling .equals(...), but line 342 does not apply the same guard before calling .name(). If it ever returns null here, this throws an NPE (still caught by the outer catch (Throwable t) and rolled back correctly) instead of the specific, actionable "commit id mismatch" diagnostic this code path is designed to surface.
🛡️ Proposed fix for the null guard
final String expectedCommitId = commit.expectedCommitId();
if (expectedCommitId != null) {
- final String actualCommitId = neo.commitIdDatabase().get(revision).name();
+ final ObjectId actualCommitObjectId = neo.commitIdDatabase().get(revision);
+ final String actualCommitId =
+ actualCommitObjectId != null ? actualCommitObjectId.name() : null;
if (!expectedCommitId.equals(actualCommitId)) {
throw new StorageException(
"commit id mismatch while recovering '" +📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| final String expectedCommitId = commit.expectedCommitId(); | |
| if (expectedCommitId != null) { | |
| final String actualCommitId = neo.commitIdDatabase().get(revision).name(); | |
| if (!expectedCommitId.equals(actualCommitId)) { | |
| throw new StorageException( | |
| "commit id mismatch while recovering '" + | |
| projectRepositoryName(repositoryName) + "' at " + revision + " (expected: " + | |
| expectedCommitId + ", actual: " + actualCommitId + "). Revisions up to " + | |
| resetToRevision + " may have diverged, or the content is not reproducible " + | |
| "byte-identically (e.g. written by a content transformer); the repository " + | |
| "was rolled back."); | |
| } | |
| } | |
| final String expectedCommitId = commit.expectedCommitId(); | |
| if (expectedCommitId != null) { | |
| final ObjectId actualCommitObjectId = neo.commitIdDatabase().get(revision); | |
| final String actualCommitId = | |
| actualCommitObjectId != null ? actualCommitObjectId.name() : null; | |
| if (!expectedCommitId.equals(actualCommitId)) { | |
| throw new StorageException( | |
| "commit id mismatch while recovering '" + | |
| projectRepositoryName(repositoryName) + "' at " + revision + " (expected: " + | |
| expectedCommitId + ", actual: " + actualCommitId + "). Revisions up to " + | |
| resetToRevision + " may have diverged, or the content is not reproducible " + | |
| "byte-identically (e.g. written by a content transformer); the repository " + | |
| "was rolled back."); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@server/src/main/java/com/linecorp/centraldogma/server/internal/storage/repository/git/GitRepositoryManager.java`
around lines 340 - 352, Update the expectedCommitId validation in the recovery
method to safely handle commitIdDatabase().get(revision) returning null before
calling name(). Represent the actual commit ID as null when no commit exists,
then reuse the existing mismatch comparison and StorageException diagnostic so
missing commits produce the intended commit-ID mismatch error instead of an NPE.
| @SuppressWarnings("UncommentedMain") | ||
| public static void main(String[] args) throws IOException { | ||
| final CentralDogma server1 = newServer(1, PORT1); | ||
| final CentralDogma server2 = newServer(2, PORT2); | ||
| // A two-node quorum needs both peers; start them concurrently. | ||
| final var start1 = server1.start(); | ||
| final var start2 = server2.start(); | ||
| start1.join(); | ||
| start2.join(); | ||
| scaffold(); | ||
| Runtime.getRuntime().addShutdownHook(new Thread(() -> { | ||
| server1.close(); | ||
| server2.close(); | ||
| })); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Close both servers when scaffolding fails.
The shutdown hook is registered only after scaffold(). If authentication, project creation, or either push fails, the started replicas have no explicit cleanup path and can leave the Gradle JavaExec process hanging. Install cleanup before scaffolding and close both servers on startup/scaffold failure.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@webapp/javaTest/java/com/linecorp/centraldogma/webapp/ReplicatedShiroCentralDogmaTestServer.java`
around lines 61 - 74, Update the main startup flow in
ReplicatedShiroCentralDogmaTestServer.main so cleanup for server1 and server2 is
installed before scaffold() runs, and ensure startup or scaffolding failures
close both servers immediately. Preserve the existing shutdown-hook cleanup for
normal process termination while preventing failed initialization from leaving
either replica running.
| // Recovery rewrites the repository history on the other replicas. | ||
| invalidatesTags: ['Repo'], |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Invalidate cached repository contents after recovery.
Recovery rewrites commit history, but this mutation invalidates only Repo. The existing getFiles, getFileContent, and getHistory queries provide the separate File tag, so subscribed or recently cached views can continue showing pre-recovery data.
Proposed fix
- invalidatesTags: ['Repo'],
+ invalidatesTags: ['Repo', 'File'],📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Recovery rewrites the repository history on the other replicas. | |
| invalidatesTags: ['Repo'], | |
| // Recovery rewrites the repository history on the other replicas. | |
| invalidatesTags: ['Repo', 'File'], |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@webapp/src/dogma/features/api/apiSlice.ts` around lines 501 - 502, Update the
recovery mutation’s invalidatesTags configuration to invalidate both the
existing Repo and File tags, ensuring getFiles, getFileContent, and getHistory
caches refresh after repository history is rewritten.
…ecovery Motivation: Ten review agents converged on two defects in the recovery feature that a green `:server:test` run could not see. A recovery is applied on a `repositoryWorker` thread, and `repositoryWorker` is a fixed 16-thread pool. The replay loop blocked on `neo.commit().join()` (up to 10,000 times) and the rollback path blocked on `internalClose()` — both of which queue a task back to that same pool, and both while holding the write lock of the repository being replaced. `GitRepository.readLock()` parks before it checks `closePending`, so concurrent readers of that repository park on the lock and consume workers instead of failing fast. Sixteen of them exhaust the pool, the queued commit never runs, and the server wedges — replication replay included. This is the same class of bug that hung CI for an hour on the purge path. Separately, the apply path re-checked the read-only precondition by reading the status storage. That status is written under a different ZooKeeper execution path than the recovery command, so an operator making the repository writable again races the replay: replicas disagree on the check, and a replica that fails it skips the log entry permanently and stays diverged in silence — the exact failure the feature exists to repair. Modifications: - Replay commits and close repositories on the calling thread during a recovery (`GitRepository.blockingCommit`, `closeInline`), so no worker thread ever waits on the pool it runs in. - Drop the apply-time read-only check. The recovery is a pure function of its payload, so every replica reaches the same state; the precondition is enforced once, at origination, where it is a decision and not a race. - Poison the repository instance when a rollback fails, instead of leaving it serving reads against a half-rewritten commit-id database. - Skip the caching-wrapper swap when a replica is already converged, so a recovery no longer cold-starts the cache of every healthy replica. - Require `ReplayCommit.expectedCommitId`; it gated the whole convergence check, which silently vanished when it was absent. - Read the repository head under the read lock and off the event loop (`Repository.head()`, `@Blocking`), so `GET .../head` cannot report a revision and a commit ID from different commits. - Build recovery payloads on a dedicated thread rather than `ForkJoinPool.commonPool()`, whose parallelism is 1 on a 2-core box. - Make the new `RepositoryManager` methods `default` rather than abstract, and count UTF-8 bytes when estimating the payload size. - Fix the verification script: the token placeholder was an unquoted bash redirect, and an unreachable or unauthorized replica printed nothing, which read exactly like a converged one. It now prints REQUEST FAILED. - Enable CodeRabbit on non-default base branches. Result: - Recovering a repository no longer risks deadlocking the server. - A recovery converges every replica, or fails loudly on it; it can no longer leave one silently diverged. - A new test drives a recovery through a single-thread pool and deadlocks on the old code (a `ForkJoinPool` hides the bug by spawning a compensation thread on `join()`). `@Timeout` on the recovery suites turns a future hang into a named failure rather than a killed CI job.
Motivation:
Driving the recovery page in a browser against a two-replica cluster showed
the generated verification script seeding both replicas with the same address:
REPLICAS='1=127.0.0.1:36462 2=127.0.0.1:36462'
The replica roster carries no HTTP port — ZooKeeper only knows the quorum
ports — so every address takes the port of the URL the page was served on.
Where replicas share a host, that collapses them onto one address. The script
then polls one server twice, gets one commit ID twice, and reads as converged
while never contacting the other replica. That is the same false pass the
script was just fixed to stop giving.
Neither the code review nor the unit tests caught it; it was only visible on
screen.
Modifications:
- Warn in the script when two replicas are seeded with the same address, and
say what to correct before running it.
- Have the script check for itself, so the warning cannot be skimmed past: it
reports any address it polled more than once.
Result:
The check can no longer claim a convergence it did not verify. Covered by
tests for both the collided and the distinct roster.
Motivation:
Central Dogma can scope a replication failure to a single repository or project and put it into read-only mode (#1305, #1327), but once a repository has diverged across replicas there is no way to repair it: the replication log cannot move a git ref backward, and old entries are trimmed away. The only recovery today is to re-sync a replica by hand.
Modifications:
An operator designates one replica as the single source of truth for a repository and a start revision; every other replica resets its git repository and commit-id database to just before that revision and replays the source's commits through to the source's head, so all replicas reconverge on identical commit IDs. The source repository is never modified. Replicated (ZooKeeper) mode only.
RecoverRepositoryCommand, which carries the commits to replay. EachReplayCommitholds the original author, timestamp, summary, detail, markup, a self-contained change set and the expected commit ID of its revision, so every replica reconstructs byte-identical commits.RepositoryManager.recoverRepository(): force-move the git head back (newGitRepository.doForceRefUpdate()), reopen the repository so the commit-id database rebuilds from the git HEAD, replay the carried commits while asserting each commit ID, and swap the repository instance. Any mismatch rolls back. An already-converged repository (the source, or a healthy replica) is left untouched, which also makes recovery idempotent.RecoverRepositoryRequestCommandasks the source, which reacts off the replay thread and originates the actual command.POST /api/v1/projects/{projectName}/repos/{repoName}/recover(system administrator only). It is rejected unless the server is replicated, the source server ID belongs to the cluster, and the repository is file-based, not internal, and read-only, so that no commit can race the recovery. The read-only precondition is re-verified when the command is applied, so a repository made writable mid-flight aborts the recovery instead of silently discarding the commits that landed. AddGET /api/v1/replicas, which returns the cluster roster.Result:
Follow-ups: